Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ install with pip - alternate proposal #34

Closed
wants to merge 6 commits into from

Conversation

oprypin
Copy link
Contributor

@oprypin oprypin commented Dec 5, 2023

Alternative to #30 - mostly untested for now, early code.

Please look at it commit by commit.
In particular this commit shows the very core of the idea:
oprypin@1d5f100

why don't we leave the complexity of 'pip-sync' behind and make a fresh subclass that knows nothing about pip-sync and doesn't need to replace many methods in a complicated way

I'm honestly so proud of this approach, the only horrible thing is that the object replaces its own __class__ on the fly. But I'm pretty sure this will just work with no problems.

oprypin and others added 6 commits December 5, 2023 02:24
Co-authored-by: Justin Flannery <juftin@juftin.com>
Co-authored-by: Justin Flannery <juftin@juftin.com>
Co-authored-by: Justin Flannery <juftin@juftin.com>
Comment on lines +63 to +66
if install_method == "pip":
self.__class__ = PipCompileEnvironmentWithPipInstall
elif install_method == "pip-sync":
self.__class__ = PipCompileEnvironmentWithPipSync
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea here with two subclasses that know how to do the right thing. My only hesitation about the approach is I've never overridden __class__ at __init__. It seems similar to how I've seen the __new__ method be used though instead of __init__.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I actually have never done or heard of this 😂 Well I have replaced __class__ but not from inside the object itself.

__new__ is definitely the way to go if it works. The big difficulty here is that in that case you don't have self.config yet so maybe this parameter would need to be scooped out of the passed config in its , *args, **kwargs). At which point I figured that became messier than this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other option I was considering is to provide 2 separate plugins in 1 package but that can be confusing

Copy link
Owner

@juftin juftin Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you can see by the lock.PipCompileLock class that I'm a big fan of dependency injection. That's how I was thinking about this when I saw the use of __class__.

def __init__(*args, **kwargs) -> None:
    super().__init__(*args, **kwargs)
    self.installer = PipCompileEnvironmentWithPipInstall(environment=self)
    
def install_project(self) -> None:
    self.installer.install_project()

Copy link
Owner

@juftin juftin Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you think about something like this: oprypin/hatch-pip-compile@pip-split...juftin:feat/pip-split/dependency-injection

My idea here is that the installer attribute gets direct access to the environment itself. Same general idea of making the PipCompileEnvironment class impartial to how its dependencies get installed but without overriding the insternal __class__ attribute

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I like these changes - that's a good API design. I will run some testing and confirm a few things. I'm excited for for some automated tests soon (starting with #18).

As far as merging goes I was going to just open a PR from pip-split2 to main and squash merge it. Happy to do that differently if you prefer.

Similarly, I'm super proud and excited about the work from this feature. I really feel like this thing is ready for prime-time now that we support these different install options. Thank you so much for all the thought and contributions on everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, it's fun and useful for me 😊

open a PR from pip-split2 to main and squash merge it

Sounds good! And no further comments, assuming this code actually works correctly

Copy link
Owner

@juftin juftin Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting a strange error where python -m piptools compile... is getting sent to the wrong interpreter and the error raised is:

/Users/justinflannery/.pyenv/versions/3.11.4/bin/python: No module named piptools

I confirmed that piptools is available in the virtualenv - it's just picking up the wrong python

Edit: This commit here is where that behavior changed e5d9c50, environment.safe_activation() was doing some nice Python environment solving for us

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the environment.safe_activation() issue is resolved I'm seeing another error surfaced in this commit: e5d9c50

diff

diff --git a/hatch_pip_compile/plugin.py b/hatch_pip_compile/plugin.py
index 9fb8282..17d1cf5 100644
--- a/hatch_pip_compile/plugin.py
+++ b/hatch_pip_compile/plugin.py
@@ -107,12 +107,13 @@ class PipCompileEnvironment(VirtualEnvironment):
         Run pip-compile if necessary
         """
         if not self.lockfile_up_to_date:
-            self.install_pip_tools()
-            if self.piptools_lock_file.exists():
-                _ = self.piptools_lock.compare_python_versions(
-                    verbose=self.config.get("pip-compile-verbose", None)
-                )
-            self.pip_compile_cli()
+            with self.safe_activation():
+                self.install_pip_tools()
+                if self.piptools_lock_file.exists():
+                    _ = self.piptools_lock.compare_python_versions(
+                        verbose=self.config.get("pip-compile-verbose", None)
+                    )
+                self.pip_compile_cli()

     def pip_compile_cli(self) -> None:
         """
diff --git a/pyproject.toml b/pyproject.toml
index 12aecd3..ccc1e46 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -67,6 +67,8 @@ python = ["3.8", "3.9", "3.10", "3.11", "3.12"]

 [tool.hatch.envs.default]
 pip-compile-constraint = "default"
+pip-compile-verbose = true
+pip-compile-installer = "pip-sync"
 post-install-commands = [
   "- pre-commit install"
 ]

Issue

Usage: python -m piptools sync [OPTIONS] [SRC_FILES]...
Try 'python -m piptools sync -h' for help.

Error: Invalid value for '[SRC_FILES]...': Path '/Users/justinflannery/git/juftin/hatch-pip-compile/requirements/requirements-docs.txt' does not exist.

Steps to Reproduce

set pip-compile-installer to pip-sync

rm -rf .venv/ requirements/ requirements.txt
hatch run docs:serve

The default lockfile is created and installed but pip-sync is called on the docs lockfile before it can be created

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay 😅 see #35

There are two small changes that resolve the issues I mentioned in the thread

@juftin
Copy link
Owner

juftin commented Dec 6, 2023

Closed in favor of #36

@juftin juftin closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants